Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libled: Add more API details #160

Merged
merged 1 commit into from
Aug 30, 2023
Merged

libled: Add more API details #160

merged 1 commit into from
Aug 30, 2023

Conversation

tasleson
Copy link
Contributor

Clarify that the "path" input variable used in led_set and led_is_management_supported need to be used with output from the function led_device_name_lookup.

We may want to consider moving this functionality internally to the library, so that the API caller is not required to make this call first. The only downside would be some additional overhead which would be negligible.

@mtkaczyk
Copy link
Contributor

Clarify that the "path" input variable used in led_set and led_is_management_supported need to be used with output from the function led_device_name_lookup.

The true is that it may not be a result of led_device_name_lookup if you know what you are doing.
it is just a kind of "realpath", so it ensures you that path is correct but it is skippable if you know exactly what form is supported.
I'm fine with documenting it but I would like to make softer requirement "this should be the result of led_device_name_lookup"

We may want to consider moving this functionality internally to the library, so that the API caller is not required to make this call first. The only downside would be some additional overhead which would be negligible.

The one advantage of current solution I see is that the user may lookup the devices one time and then use saved values. I think that we can maintain both solutions.

If you would like to have it build-in in led_set, we can make it configurable, by flag bool lookup_device (or mask, to be ready for more settings like this?):

led_status_t led_set(struct led_ctx *ctx, const char *path, enum led_ibpi_pattern ibpi, bool lookup_device)

Even if overhead is negligible, in general API will be used to call led_set so I would like to avoid this overhead. We will need to additionally allocate some memory which may not be needed in some cases.
What do you think?

@tasleson
Copy link
Contributor Author

The true is that it may not be a result of led_device_name_lookup if you know what you are doing. it is just a kind of "realpath", so it ensures you that path is correct but it is skippable if you know exactly what form is supported. I'm fine with documenting it but I would like to make softer requirement "this should be the result of led_device_name_lookup"

Updated

Even if overhead is negligible, in general API will be used to call led_set so I would like to avoid this overhead. We will need to additionally allocate some memory which may not be needed in some cases. What do you think?

Let's just leave it as is. It will be very apparent to any API user that things aren't correct if they neglect to call the lookup first.

Clarify that the "path" input variable used in led_set and
led_is_management_supported need to be used with output from
the function led_device_name_lookup.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
@mtkaczyk mtkaczyk merged commit 77f5a5c into intel:master Aug 30, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants